-
Notifications
You must be signed in to change notification settings - Fork 416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add contributing guidelines #875
Conversation
This is just a draft. Feel free to submit your ideas @majorgreys @brettlangdon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to have a section explaining that ddtrace.internal
should only be used within the ddtrace
module namespace, that breaking API changes can happen at any time to that module, etc etc.
We might also want to consider adding a GitHub issue/pull request template. Can include some basic things like "did you enable debug logging", can you give us a pip freeze
, etc etc.
CONTRIBUTING.md
Outdated
chance that the implementation appears doable in several steps. In order to | ||
facilite the review process, we strongly advise to split your feature | ||
implementation in small pull requests (if that is possible) so they contain a | ||
very small number of commits (a single commit per pull request being optimal). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not too concerned with number of commits in a PR, we do squash merging anyways. Keeping emphasis on "small PRs" is 👍 though!
CONTRIBUTING.md
Outdated
That ensures that: | ||
|
||
1. Each commit passes the test suite. | ||
2. The human review process is easier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this confusing? I've added a explanation.
CONTRIBUTING.md
Outdated
|
||
## Python Versions Support | ||
|
||
Python versions that are supported are the Python version that are supported by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably actually fully list out all supported versions here.
This way we can be explicit about it being CPython only, and in the off case we need to support a non-community supported version for a customer, then we can potentially list it here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added CPython, though I'm a bit reluctant to have the list of version supported listed here, 'cause it'll make us need to maintain and update it.
That's what I meant by "supported by the community". e.g. Python 3.4 is EOL.
Maybe it's unclear the way it's written?
CONTRIBUTING.md
Outdated
Python versions that are supported are the Python version that are supported by | ||
the community. | ||
|
||
## Libraries Support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this section, we might want to loosen the wording here a bit. I don't want customers to think we will YOLO just drop support for a given library or version. We want to make sure they are confident in using our library, that using it will "just work".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've enhanced this a bit.
Yeah, I don't have enough experience yet that I feel confident about written an issue template, but if you have a common list of questions you know you have to ask on each issue, feel free to list them somewhere. I can then start another PR :) |
Moved to RST, updated workflow |
Any reason we don't merge this @DataDog/apm-python ? |
No description provided.